bug: add handler for handling changed domain #500
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 tasks
slint
reviewed
Nov 6, 2024
utnapischtim
reviewed
Nov 7, 2024
lnielsen
reviewed
Nov 7, 2024
invenio_accounts/datastore.py
Outdated
| @@ -73,11 +73,16 @@ def commit(self): | |||
|
|
|||
| def mark_changed(self, sid, uid=None, rid=None, model=None): | |||
| """Save a user to the changed history.""" | |||
| # needed so that we have the id from the DB on the model | |||
| self.db.session.flush() | |||
There was a problem hiding this comment.
@slint Can you have a look at the impact of having a flush here.
There was a problem hiding this comment.
I see we very often tiptoe around flushes, and get them in implicitly just by chance because we perform another SELECT query down the line, e.g.:
domain = Domain.create(...)
# ...in another function...
user = UserAggregate.get(...) # <-- this triggered a flush, so our `domain`
# from above now has a `domain.id`So I think it's better to be explicit about it, which is what calling mark_changed also implies (i.e. that something will be persisted to the DB).
@carlinmack is also checking this change on tests across other repos to see if there are wrong expectations.
carlinmack
force-pushed
the
commit-domain
branch
from
31ddca0 to
d37027a
Compare
remove setting of role.id as the added flush() will add the ID from DB
carlinmack
force-pushed
the
commit-domain
branch
2 times, most recently
from
6f9ad3c to
b8ae274
Compare
carlinmack
force-pushed
the
commit-domain
branch
from
b8ae274 to
72e7cf1
Compare
carlinmack
commented
Nov 12, 2024
carlinmack
force-pushed
the
commit-domain
branch
from
a2e8256 to
4644611
Compare
zubeydecivelek
approved these changes
Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
❤️ Thank you for your contribution!
(Part of) Close inveniosoftware/invenio-app-rdm#2777
Description
We call
mark_changedbut we did not check that it handled DomainsCalling flush as similarly done in the pre-commit hook
Also remove setting of role.id as the added flush() will add the ID from DB
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: